Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Dubbo-issue-2178] Please consider supporting Java 8 Date/Time type when serializing with Kryo #2178 #2210

Closed

Conversation

daryl666
Copy link

@daryl666 daryl666 commented Aug 7, 2018

What is the purpose of the change

Please consider supporting Java 8 Date/Time type when serializing with Kryo #2178

Brief changelog

add the if statement of java8 date/time when getDeclaredConstructor

Verifying this change

org.apache.dubbo.common.serialize.kryo.CompatibleKryo

From Baiji, Term 268, Team 3, Issue #2187, Q3-4

@daryl666 daryl666 changed the title [Dubbo-common] Please consider supporting Java 8 Date/Time type when serializing with Kryo #2178 [Dubbo-issue-2178] Please consider supporting Java 8 Date/Time type when serializing with Kryo #2178 Aug 7, 2018
@@ -34,7 +38,7 @@ public Serializer getDefaultSerializer(Class type) {
throw new IllegalArgumentException("type cannot be null.");
}

if (!type.isArray() && !type.isEnum() && !ReflectionUtils.checkZeroArgConstructor(type)) {
if (!type.isArray() && !type.isEnum() && !type.equals(LocalDate.class) && !type.equals(LocalDateTime.class) && !type.equals(LocalTime.class) && !ReflectionUtils.checkZeroArgConstructor(type)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Simply check types will not solve the problem.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I've checked that this works, and kryo has internally registered some Serializers for Java 8 Time types.

But we still have a problem, we cannot list all possible classes here.

@chickenlj
Copy link
Contributor

See #2220

@chickenlj chickenlj closed this Aug 9, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants